-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DEL.X / DEL.R ops #164
DEL.X / DEL.R ops #164
Conversation
sync master changes
static void delay_common_add(scene_state_t *ss, exec_state_t *es, | ||
int16_t i, int16_t delay_time, | ||
const tele_command_t *post_command) { | ||
ss->delay.count++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would add a check here to make sure it doesn't exceed DELAY_SIZE
- imho it's a good idea to have safety checks in helper functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah good catch I can add that in. on the same topic do you think it would be good to move tele_has_delays
into the helper function as well? the downside is it would get called excess times times by DEL.X
, but the change would make the helper function slightly more robust for other future usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd leave it out - the function has a well defined and logical contract (it adds a delay), tele_has_delays
is outside of that (and only has meaning to where this function is called from).
other than the 1 comment looks good! |
no I didn't run clang-format. I'll do that after making the change you suggested and then update the pull. |
re: clang - check this: https://github.com/monome/teletype#code-formatting you'll need to install clang-format first. if you're on windows you might also need to modify the make file (or you could prob run it from bash) |
I ran clang-format using here's the |
@alpha-cactus that’s weird... i ran it before i did a pull request for 3.0, so there shouldn’t be any major changes. make sure it’s using the clang config included in the repo. if it does just leave it out for now and i’ll investigate - entirely possible i used the wrong config.. |
thanks for confirming! i'll take a look then, if i did it wrong on my side i'll need to fix several files. we can just skip the step for this PR. |
src/ops/delay.c
Outdated
delay_time_next = normalise_value(1, 32767, 1, delay_time_next); | ||
|
||
num_delays--; | ||
i++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, should've looked into the code closer the 1st time.. there is a problem with this spot here. for multiple delays it will find the 1st available slot but for any consecutive delays it will use the next delay without checking if it's free or not. you have to check that for each delay added.
this: while (ss->delay.time[i] != 0 && i != DELAY_SIZE) i++;
can be moved to delay_common_add
and i
parameter can be removed, so the function's responsibility will be finding the 1st available spot and adding a delay there. it could return a bool indicating if it was added, so if false
is returned this loop can assume no more spots are available and exit early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow another good catch. elegant solution too. I'll work on getting that together tonight.
… delay op functions updated accordingly
// Be careful not to set delay.time[i] to 0 before calling this function. | ||
while (ss->delay.time[i] != 0 && i != DELAY_SIZE) i++; | ||
|
||
if (delay_time < 1) delay_time = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a check for the delay time to the common code function. removed the check from the DEL
op function, but it's still in the DEL.X
and DEL.R
because the wrap check would cause 32ms delays if the delay time param was negative.
|
||
while ( num_delays > 0 && delay_common_add(ss, es, delay_time_next, post_command) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the function call is part of the while check instead of using a separate bool variable. pretty sure this is fine, but I may be missing something.
What does this PR do?
added
DEL.X
andDEL.R
ops allowing for chained(multiple) delays at spaced intervals. added common delay add function for all delay ops to save code space. additionally increased the maximum number of delays from 8 to 16.Provide links to any related discussion on lines.
https://llllllll.co/t/teletype-3-feature-requests-and-discussion/16219/27?u=alphacactus
How should this be manually tested?
tested both old and new delay ops with variable delay times and number of delays. tested edge cases such as calling
DEL.X
for zero delays, delay times of zero, and negative delay times.Any background context you want to provide?
If the related Github issues aren't referenced in your commits, please link to them here.
I have,